feat(collaboration): portable snapshots and navigable map comments - #1753
Conversation
A guest could join a session and see an empty map: snapshots kept local file references and control-managed vector data the collaborator cannot read, so shared layers now embed their features and drop the reloadable flag. Guests also arrive at the host's viewport instead of waiting for the host's next move, and the session Copy button yields a joinable URL. Comments become reachable from the map: clicking a pin reveals, highlights and scrolls to its card, the panel ships collapsed on the Style rail so it is discoverable, comments are saved with the project, and "C" places a new one from the command palette.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change updates comment selection and activation, panel registration, portable asynchronous collaboration snapshots, local project-state merging, separate comment synchronization, and configurable SQLite-backed relay storage. ChangesComments and collaboration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CommentMapOverlay
participant DesktopShell
participant CommentsPanel
participant CommentThread
CommentMapOverlay->>DesktopShell: select comment ID
DesktopShell->>CommentsPanel: pass selectedCommentId
CommentsPanel->>CommentsPanel: reveal filter and scroll card
CommentsPanel->>CommentThread: pass selected state
sequenceDiagram
participant useCollaboration
participant buildCollaborationSnapshot
participant CollaborationRelay
participant LocalProject
useCollaboration->>buildCollaborationSnapshot: build asynchronous snapshot
buildCollaborationSnapshot->>CollaborationRelay: send portable redacted snapshot
CollaborationRelay-->>useCollaboration: return inbound project
useCollaboration->>LocalProject: merge local view and plugin state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/comments/CommentsPanel.tsx`:
- Around line 343-345: Replace the literal tooltip in the session-copy control
using the component’s react-i18next t() function, add the corresponding English
source entry to src/i18n/locales/en.json, and preserve the existing “Copy
session URL” text as the translation value.
In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts`:
- Around line 81-87: Update sendSnapshot to catch rejections from
buildCollaborationSnapshot, ignoring failures from stale requests identified by
snapshotRequestRef.current. For the active request, preserve the existing
edit/pause checks and report the construction failure through the collaboration
state error mechanism instead of allowing an unhandled rejection.
In `@apps/geolibre-desktop/src/lib/build-project-snapshot.ts`:
- Around line 73-79: Update the snapshot-building flow around state,
materializeEmbeddableVectorLayers, and prepareCollaborationLayers to capture the
layer array once and verify the store still contains that same revision after
materialization. If the layers changed during the await, retry or discard the
build rather than combining materialized data with newer layers; ensure the
resulting snapshot always uses one consistent layer revision.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ab64987f-4875-4c53-b9f3-46d2a799d4b3
📒 Files selected for processing (12)
apps/geolibre-desktop/src/components/comments/CommentMapOverlay.tsxapps/geolibre-desktop/src/components/comments/CommentThread.tsxapps/geolibre-desktop/src/components/comments/CommentsPanel.tsxapps/geolibre-desktop/src/components/layout/DesktopShell.tsxapps/geolibre-desktop/src/components/layout/SettingsDialog.tsxapps/geolibre-desktop/src/components/layout/TopToolbar.tsxapps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/hooks/useProjectFileActions.tsapps/geolibre-desktop/src/hooks/useRegisterCommentsPanel.tsapps/geolibre-desktop/src/lib/build-project-snapshot.tsapps/geolibre-desktop/src/lib/collaboration-layers.tstests/collaboration-snapshot.test.ts
💤 Files with no reviewable changes (1)
- apps/geolibre-desktop/src/components/layout/SettingsDialog.tsx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
All four inline comments posted successfully. Code reviewBugs
Performance
Quality
Security
CLAUDE.md
|
🔍 GitHub Pages PR preview
|
A guest joining a brand new session saw nothing until the host happened to edit: the store subscription only observes changes made after attach, so a project loaded before starting collaboration was never broadcast. The host now seeds a snapshot when the relay has none. Plugin activation and settings are participant-local. Applying a peer's activePluginIds deactivated local controls and let their teardown remove the native layers they own, so inbound snapshots keep the local plugin state; plugin-created layers still travel as portable GeoJSON. Comments stay out of projectChanged because the comment-mutation protocol already syncs them, and answering one with a whole-project snapshot let a comment overwrite unrelated layer state. Embedding that portable GeoJSON outgrew both the 1 MB snapshot cap (now 10 MB, configurable per deployment via COLLAB_MAX_SNAPSHOT_BYTES) and the Durable Object 2 MiB key/value entry limit, so the Cloudflare relay stores the project in a SQLite row and falls back to the legacy key for sessions created before it.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/geolibre-desktop/src/hooks/useCollaboration.ts (1)
146-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend the initial host presence after
join.
attach()callsbindPresence()before it sendsjoin.bindPresence()immediately sends apresenceframe. The relay rejects that frame because the socket has no join attachment yet.As a result,
message.presencehas no host view during welcome. Lines 158-162 then leave the guest at its local viewport until the host moves.Proposed fix
- const map = mapControllerRef.current?.getMap() ?? null; - const detachMap = map ? bindPresence(map, conn) : () => {}; - conn.send({ type: "join", clientId: selfIdRef.current ?? crypto.randomUUID(), displayName, color, hostToken, }); + + const map = mapControllerRef.current?.getMap() ?? null; + const detachMap = map ? bindPresence(map, conn) : () => {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts` around lines 146 - 162, The collaboration flow must send the host’s initial presence only after the guest has joined and the relay attachment exists. Update the join/welcome handling around attach(), bindPresence(), and the message.role === "guest" host-view application so the host presence is sent after join rather than during premature bindPresence(), allowing message.presence to contain the host view immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/collaboration-project.test.ts`:
- Around line 10-14: Update both fixtures in tests/collaboration-project.test.ts
(lines 10-14) and tests/collaboration-project-changed.test.ts (lines 88-91) to
include the required ProjectPluginState fields mapControlPositions and
activePluginIds; add settings to the second fixture as well, using empty values
where those tests do not depend on them.
---
Outside diff comments:
In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts`:
- Around line 146-162: The collaboration flow must send the host’s initial
presence only after the guest has joined and the relay attachment exists. Update
the join/welcome handling around attach(), bindPresence(), and the message.role
=== "guest" host-view application so the host presence is sent after join rather
than during premature bindPresence(), allowing message.presence to contain the
host view immediately.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9dd24599-6413-4e0c-99cf-29d0ab10d514
📒 Files selected for processing (8)
apps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/lib/collaboration-project.tsapps/geolibre-desktop/src/lib/project-broadcast-changed.tsdocs/collaboration.mdpackages/collab-core/src/session.tstests/collaboration-project-changed.test.tstests/collaboration-project.test.tsworkers/collab/src/session.ts
- buildCollaborationSnapshot: read the layer array once and feed it to both materialization and prepareCollaborationLayers, so a layer added during the await can no longer lose localFileReloadable without gaining features. - buildProjectSnapshot takes an optional layers override, removing the second copy of the ~20-field projectFromStore call a new project field could miss. - sendSnapshot catches a failed snapshot build and reports it through collaboration state instead of leaving an unhandled rejection. - SettingsDialog: restore collapseRightPanel when re-enabling the Comments panel, matching the Browser toggle and the panel's own mount state. - Localize the session copy tooltip via the existing collaborate.copyLink key.
- Complete the ProjectPluginState fixtures in the collaboration tests: both were missing mapControlPositions, and the projectChanged fixture was also missing activePluginIds and settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/hooks/useCollaboration.ts`:
- Around line 93-95: Update the error-setting block for the current snapshot
request to set collaborate.shareFailed only when !syncPausedRef.current and
canEdit() are both true, preserving the size-specific relay error after
synchronization is paused.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7e271b6-f5ad-4b3d-9024-213f0e512100
📒 Files selected for processing (5)
apps/geolibre-desktop/src/components/comments/CommentsPanel.tsxapps/geolibre-desktop/src/components/layout/SettingsDialog.tsxapps/geolibre-desktop/src/hooks/useCollaboration.tsapps/geolibre-desktop/src/i18n/locales/en.jsonapps/geolibre-desktop/src/lib/build-project-snapshot.ts
| // portable and arrives empty. A layer added after this read is simply absent | ||
| // from this snapshot; the store change that added it schedules the next one. | ||
| const source = useAppStore.getState().layers; | ||
| const materialized = await materializeEmbeddableVectorLayers(source); |
There was a problem hiding this comment.
Every debounced snapshot re-materializes all embeddable local vector layers, even when the change that triggered the broadcast is unrelated.
materializeEmbeddableVectorLayers re-queries DuckDB (control.getLayerGeoJSON) for every embeddable local/control-managed vector layer on every call to buildCollaborationSnapshot. Since the prepared/portable layers computed here are never written back into the store (useAppStore's layers stays the original, non-portable version), the next store change — even something as unrelated as basemapOpacity or legend — schedules another sendSnapshot() that redoes the full materialization from scratch for every such layer.
With the snapshot cap now raised from 1 MB to 10 MB (packages/collab-core/src/session.ts), this could mean repeatedly re-serializing multi-MB feature collections from DuckDB on every minor, unrelated project edit while a session with embedded local data is active.
Confidence: medium-high — clear from the code structure; actual impact depends on how large/frequent the embedded local datasets are in practice.
There was a problem hiding this comment.
Not changing this, leaving the thread open for a maintainer call (same concern as the earlier thread on useCollaboration.ts).
The cost is real but a cache here trades a correctness property for it. materializeEmbeddableVectorLayers only touches layers matching isEmbeddableLocalVectorLayer, so a project without local/control-managed vector layers already pays nothing. When such layers do exist, the control owns their features and can mutate them without writing to the store, so there is no store-visible revision to key a memo on. Keying on the layers array identity would mean an unrelated edit broadcasts the features as they were at the last layer-list change rather than as they are now — silently shipping stale geometry, which is worse than the redundant read.
The fix worth having is a data revision counter on the vector control, which getLayerGeoJSON callers (save and export, not just collaboration) could all use. That belongs in its own change.
Posting a comment meant leaving the keyboard for the mouse, and the form offered no hint that a shortcut existed. The button now carries the platform-correct label and aria-keyshortcuts alongside the handler.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/geolibre-desktop/src/components/comments/AddCommentDialog.tsx`:
- Around line 15-17: Update the POST_COMMENT_SHORTCUT definition to explicitly
set shift: false, ensuring matchesShortcut accepts only the advertised unshifted
Ctrl+Enter or Meta+Enter combination and rejects Shift-modified variants.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc3c8106-ebe0-4895-8fe1-739130ea0f03
📒 Files selected for processing (2)
apps/geolibre-desktop/src/components/comments/AddCommentDialog.tsxapps/geolibre-desktop/src/i18n/locales/en.json
- Store a hosted snapshot in chunks across SQLite rows. A Durable Object caps a SQLite string at 2 MB just as it caps a key/value entry, so the one-row table did not actually admit the 10 MB the cap now allows. - buildCollaborationSnapshot re-reads and re-materializes (bounded) when an edit lands mid-read, so layerGroups and selectedLayerId can no longer name a layer the broadcast does not carry. - sendSnapshot reports a build failure only while the request may still broadcast, so it cannot overwrite a relay error that paused sync. - Correct the snapshot-cap comments and docs: 32 MiB is the received-message ceiling, and note the separate 2 MB storage bound. - Update the viewer-mode shortcut comment, which no longer held once add.comment started carrying a shortcut.
- Set shift: false on POST_COMMENT_SHORTCUT. An omitted shift means "ignored" in matchesShortcut, so the dialog also posted on Ctrl/Cmd+Shift+Enter, a chord the button never advertises.
| this.ctx.storage.sql.exec( | ||
| "INSERT INTO collab_snapshot_chunks (seq, value) VALUES (?, ?)", | ||
| seq, | ||
| snapshot.slice(offset, offset + SNAPSHOT_CHUNK_CHARS), |
There was a problem hiding this comment.
Bug (medium confidence): snapshot.slice(offset, offset + SNAPSHOT_CHUNK_CHARS) splits the JSON string on raw UTF-16 code-unit boundaries. If a chunk boundary happens to fall between the two halves of a surrogate pair (e.g. an emoji in a comment author name, layer name, or an embedded GeoJSON property — plausible now that whole vector datasets get embedded), each half becomes a lone surrogate in its own chunk. Storing that as SQLite TEXT typically forces UTF-8 re-encoding of each chunk independently, which replaces a lone surrogate with U+FFFD — silently corrupting that character when the chunks are rejoined on read (readSqlSnapshot's .join("")). This only bites snapshots over 256KB (i.e. once chunking actually kicks in), so it'd be easy to miss in normal testing.
A boundary-safe split (e.g. nudging the slice point to avoid landing on a high surrogate) would avoid this.
| await this.writeSnapshot(JSON.stringify(project)); | ||
| await this.ctx.storage.put("rev", rev); |
There was a problem hiding this comment.
Reliability (low-medium confidence): this replaces what used to be a single atomic storage.put({ snapshot, rev }) with two separately-awaited operations — writeSnapshot (which itself ends in await this.ctx.storage.delete("snapshot")) followed by await this.ctx.storage.put("rev", rev). If the Durable Object is evicted or throws between these awaited steps, the stored snapshot content and the rev counter can end up out of sync (new content persisted with a stale rev, or vice versa). The within-writeSnapshot chunk delete+insert itself is fine (no await between those calls, so Cloudflare's SQLite storage coalesces them into one transaction), but the boundary between writeSnapshot and the rev put is a real gap. Worth wrapping both in this.ctx.storage.transaction() (or an explicit storage.sql transaction) if that guarantee still matters here.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Summary
activePluginIdsdeactivated local controls and let their teardown remove the native layers they own; plugin-created layers still travel as portable GeoJSON. Comments are likewise excluded fromprojectChangedbecause thecomment-mutationprotocol already syncs them.COLLAB_MAX_SNAPSHOT_BYTES) and the Cloudflare relay stores the project in a SQLite row instead of a Durable Object key/value entry, with a fallback read for sessions created before that table existed.?collab=URL rather than a bare code.Cplaces a new comment from the command palette. Marker hover scaling moved to a child element so it no longer fights MapLibre's translate transform.Test plan
npm run buildnpm run test:frontend(5473 pass, 0 fail)npm run test:workerpre-commit run --files <changed paths>Cand confirm the comment placement tool activatesSummary by CodeRabbit
New Features
Bug Fixes